Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add an env variable to enable the new rviz2 theme #1017

Merged
merged 14 commits into from
Sep 12, 2024
Merged

feat: add an env variable to enable the new rviz2 theme #1017

merged 14 commits into from
Sep 12, 2024

Conversation

KhalilSelyan
Copy link
Contributor

@KhalilSelyan KhalilSelyan commented Jun 4, 2024 •
edited by xmfcx
Loading

Description

Please see:

This just adds the required env var to the launch file to enable the new theme.

Related links

Tests performed

I've confirmed by uninstalling qt5ct I was able to run rviz with older theme with this PR enabled.

And with this enabled, and

Notes for reviewers

  • This PR should be reviewed alongside any related PRs that involve updates to the UI or styling of RViz.
  • Ensure that the setup script is thoroughly tested in different environments to confirm its robustness.

Interface changes

None.

Effects on system behavior

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@KhalilSelyan KhalilSelyan added the component:ui User interface, user experience, graphical user interfaces. (auto-assigned) label Jun 4, 2024
@KhalilSelyan KhalilSelyan self-assigned this Jun 4, 2024
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) and removed component:ui User interface, user experience, graphical user interfaces. (auto-assigned) labels Jun 4, 2024
@KhalilSelyan KhalilSelyan marked this pull request as ready for review June 5, 2024 18:00
@KhalilSelyan KhalilSelyan added component:ui User interface, user experience, graphical user interfaces. (auto-assigned) and removed type:documentation Creating or refining documentation. (auto-assigned) labels Jun 5, 2024
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) and removed component:ui User interface, user experience, graphical user interfaces. (auto-assigned) labels Jun 6, 2024
@xmfcx xmfcx marked this pull request as draft June 6, 2024 15:48
@xmfcx
Copy link
Contributor

xmfcx commented Jun 6, 2024

Please put the installation related things to the https://github.com/autowarefoundation/autoware/tree/main/ansible/roles

We don't want to run custom scripts in the CMakeLists files.

@github-actions github-actions bot removed the type:documentation Creating or refining documentation. (auto-assigned) label Jun 7, 2024
@KhalilSelyan KhalilSelyan mentioned this pull request Jun 7, 2024
Merged
7 tasks
@KhalilSelyan
Copy link
Contributor Author

Please put the installation related things to the https://github.com/autowarefoundation/autoware/tree/main/ansible/roles

We don't want to run custom scripts in the CMakeLists files.
@xmfcx I have opened a separate PR on autoware repo with the ansible scripts update
autowarefoundation/autoware#4838

@stale Stale
Copy link

stale bot commented Aug 6, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale label Aug 6, 2024
@xmfcx xmfcx force-pushed the qt5ct-dark-mode-integration branch from fb5c3bc to fd85658 Compare September 5, 2024 11:05
@xmfcx
Copy link
Contributor

xmfcx commented Sep 5, 2024

image

It looks like this for me, is this expected?

The button in the circled area looks weird for me.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 5, 2024 •
edited
Loading

image

Could you make these less subtle (smaller shadow) and (bottom-centered) (as opposed to bottom-right)?

reference:
2024-09-05_15-15

@stale stale bot removed the status:stale label Sep 5, 2024
@KhalilSelyan KhalilSelyan mentioned this pull request Sep 5, 2024
Merged
@xmfcx xmfcx force-pushed the qt5ct-dark-mode-integration branch from fd85658 to 7d024d3 Compare September 11, 2024 15:01
@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

could you make these selected line color same as the selected menu items?

image

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

Normal RViz
image

New theme
image

sorry for the screenshot quality, OS didnt let me screenshot while textbox was selected.

new theme adds an underline to the selected text in the textbox, could you remove that?

also i think there is a border around it now. but i think it will look good once it becomes gray with the comment above.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

image

this has issues

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

image

disabled things don't look disabled

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

these textbox/combobox dropdowns should also adopt the usual menu color scheme. and remove all rounded corners from them.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

image

these shouldn't have checkboxes and follow the menu color scheme. and corners are black for some reason.

@KhalilSelyan
Copy link
Contributor Author

@xmfcx Should be good to re-review after the latest changes on both the ansible qss side and the tier4_state_rviz_plugin

KhalilSelyan and others added 14 commits September 12, 2024 12:33
@KhalilSelyan @xmfcx
feat: add new SVG icon for rviz dark theme
33183c6 
Signed-off-by: KhalilSelyan <[email protected]>
@KhalilSelyan @xmfcx
feat: add new qss file for qt5ct/rviz
171e26b 
Signed-off-by: KhalilSelyan <[email protected]>
@KhalilSelyan @xmfcx
chore: add QT_QPA_PLATFORMTHEME environment variable for qt5ct
06c109b 
Signed-off-by: KhalilSelyan <[email protected]>
@KhalilSelyan @xmfcx
chore: Update QSS file and qt5ct config for rviz using script
c06b2e4 
Signed-off-by: KhalilSelyan <[email protected]>
@pre-commit-ci @xmfcx
style(pre-commit): autofix
3a70644
@KhalilSelyan @xmfcx
chore: check/install qt5ct + update style and color scheme in qt5ct c…
c5e606b 
…onfig

Signed-off-by: KhalilSelyan <[email protected]>
@KhalilSelyan @xmfcx
chore: update the baseline path for icons
0a62e3c 
Signed-off-by: KhalilSelyan <[email protected]>
@KhalilSelyan @xmfcx
fix: update the path updating inside the qss file for the icons
e3c2bc2 
Signed-off-by: KhalilSelyan <[email protected]>
@KhalilSelyan @xmfcx
feat: add post build script execution
3b8e91a 
Signed-off-by: KhalilSelyan <[email protected]>
@pre-commit-ci @xmfcx
style(pre-commit): autofix
b631b21
@KhalilSelyan @xmfcx
fix: check for qt5ct.conf first
3339560 
Signed-off-by: KhalilSelyan <[email protected]>
@pre-commit-ci @xmfcx
style(pre-commit): autofix
d3a6889
@KhalilSelyan @xmfcx
Revert all changes except addition of env QT_QPA_PLATFORMTHEME in the…
c11a5db 
… launch file, all changes will be moved to ansible playbook/roles

This reverts commit 037224b.

Signed-off-by: KhalilSelyan <[email protected]>
@pre-commit-ci @xmfcx
style(pre-commit): autofix
2ba16b9
@xmfcx xmfcx force-pushed the qt5ct-dark-mode-integration branch from 7d024d3 to 2ba16b9 Compare September 12, 2024 09:33
@xmfcx xmfcx marked this pull request as ready for review September 12, 2024 11:06
@xmfcx xmfcx changed the title feat: qt5ct dark mode integration using script feat: add env variable to enable rviz2 theme Sep 12, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Sep 12, 2024

This is just an env var for the theme, we can merge it now. It shouldn't be an issue even if the theme is not installed.

But I will test by removing qt5ct on my machine, just in case.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 12, 2024

I've confirmed by uninstalling qt5ct I was able to run rviz with older theme with this PR enabled.

@xmfcx xmfcx merged commit 42ba6ab into main Sep 12, 2024
21 of 22 checks passed
@xmfcx xmfcx deleted the qt5ct-dark-mode-integration branch September 12, 2024 11:12
@xmfcx xmfcx changed the title feat: add env variable to enable rviz2 theme feat: add an env variable to enable the new rviz2 theme Sep 12, 2024
@xmfcx xmfcx mentioned this pull request Sep 12, 2024
Closed
10 tasks
@tier4-autoware-public-bot tier4-autoware-public-bot bot mentioned this pull request Sep 12, 2024
Merged
@Autumn60 Autumn60 mentioned this pull request Nov 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yukkysaito yukkysaito yukkysaito left review comments

@xmfcx xmfcx xmfcx approved these changes

@mitsudome-r mitsudome-r Awaiting requested review from mitsudome-r mitsudome-r is a code owner

Assignees

@KhalilSelyan KhalilSelyan

Labels
run:build-and-test-differential
Projects
No open projects
Autoware Labs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KhalilSelyan @xmfcx @yukkysaito